-
Notifications
You must be signed in to change notification settings - Fork 123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Log website URL upon deployment #1329
Conversation
… jest [misc] update test case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
package-lock
(from running npm run setup
) will need to be committed too
Some comments:
packages/core/src/utils/git.js
Outdated
async getRemoteBranchFile(simpleGit, type, remote, branch, fileName) { | ||
const catFileTarget = `${remote}/${branch}:${fileName}`; | ||
return simpleGit.catFile([type, catFileTarget]) | ||
.catch((err) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since async
is used here, let's do a try ... return await ... catch
instead!
@@ -27,6 +27,11 @@ jest.mock('fs'); | |||
jest.mock('walk-sync'); | |||
jest.mock('gh-pages'); | |||
jest.mock('../../src/Page'); | |||
jest.mock('simple-git', () => () => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this mock still necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ang-zeyu otherwise, there would be tons of warnings warn: message=fatal: Not a valid object name origin/gh-pages:CNAME
thrown in the test for deployment in Site.test.js
.
packages/core/src/Site/index.js
Outdated
|
||
// https://<name|org name>.github.io/<repo name>/ | ||
function constructGhPagesUrl(remoteUrl) { | ||
if (remoteUrl.includes(HTTPS_PART)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we use startsWith
here instead?
Likewise for SSH_PART
(i'm not too sure on the format requirements here though, correct me if I'm wrong!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that would make more sense, thank you!
packages/core/src/Site/index.js
Outdated
const name = (parts[0].split(':'))[0]; | ||
return `https://${name}.${GITHUB_IO_PART}/${repoName}`; | ||
} | ||
throw new Error(`Unknown remote url ${remoteUrl}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the message here isn't used, let's just return null
directly for use in the above.
depUrl => (depUrl != null ? resolve(`Deployed at ${depUrl}!`) : resolve('Deployed!'))
Could remove the catch(..)
below as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ang-zeyu I'm not sure whether we should remove the catch(..)
statement; I put it there to deal with cases where gitUtil
fails when calling the methods of simple-git
, causing Promise.all
to reject
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's a try ... catch
in gitUtil
already though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, I've removed the throw statement; it returns null
now
packages/core/src/Site/index.js
Outdated
.then(resolve) | ||
.then(() => { | ||
const git = simpleGit({ baseDir: process.cwd() }); | ||
const options = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to move the previous options
up to avoid duplication?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can use a more explicit variable name (maybe deployOptions
?) to make it clearer what kind of options
these are as well!
packages/core/src/utils/git.js
Outdated
*/ | ||
async getRemoteBranchFile(simpleGit, type, remote, branch, fileName) { | ||
const catFileTarget = `${remote}/${branch}:${fileName}`; | ||
return simpleGit.catFile([type, catFileTarget]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not too familiar with CNAME
file and cat-file
, give me some time to take a look at that!
Perhaps one of the other maintainers @acjh @marvinchin might have across it before though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't something I'm familiar with either, unfortunately :(
packages/core/src/Site/index.js
Outdated
// git@github.com:<name|org>/<repo>.git (SSH) | ||
const parts = remoteUrl.split('/'); | ||
const repoName = parts[parts.length - 1].toLowerCase(); | ||
const name = (parts[0].split(':'))[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the ending be [1]
? would parts[0].substring(SSH_PART.length)
work instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that would work better!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking at this! Left a couple of suggestions 🙂
packages/cli/index.js
Outdated
.then(() => { | ||
logger.info('Deployed!'); | ||
}) | ||
.then(deployMsg => logger.info(deployMsg)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't feel very intuitive for the Site
constructor to return a message. The message feels more like the responsibility of the CLI. I think that it might be more appropriate for it to return some data about the site created (the url in this case), which we can then use to construct the deploy message here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, I'll do that!
packages/core/src/Site/index.js
Outdated
.then(resolve) | ||
.then(() => { | ||
const git = simpleGit({ baseDir: process.cwd() }); | ||
const options = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can use a more explicit variable name (maybe deployOptions
?) to make it clearer what kind of options
these are as well!
packages/core/src/Site/index.js
Outdated
options.branch = this.siteConfig.deploy.branch || defaultDeployConfig.branch; | ||
return Site.getDeploymentUrl(git, options); | ||
}) | ||
.then(depUrl => (depUrl != null ? resolve(`Deployed at ${depUrl}!`) : resolve('Deployed!'))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use !==
instead of !=
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't notice I put !=
, thanks!
packages/core/src/Site/index.js
Outdated
} | ||
|
||
const { remote, branch, repo } = options; | ||
const cnamePm = gitUtil.getRemoteBranchFile(git, 'blob', remote, branch, 'CNAME'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm missing some context here, but I'm not sure what Pm
stands for. Is it for promise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is, I'd prefer to avoid using uncommon (to me, at least! if this is a convention you are more familiar with please correct me) abbreviations to avoid confusion when future devs look at this variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was meant to be shorthand for 'Promise'; Ill rename it to cnamePromise
for better readability. Thanks!
packages/core/src/utils/git.js
Outdated
*/ | ||
async getRemoteBranchFile(simpleGit, type, remote, branch, fileName) { | ||
const catFileTarget = `${remote}/${branch}:${fileName}`; | ||
return simpleGit.catFile([type, catFileTarget]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't something I'm familiar with either, unfortunately :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- remote logger.warn from gitUtils - trim whitespaces and line endings from cname
okay, I've made the changes as requested! |
packages/cli/package-lock.json
Outdated
@@ -4,3052 +4,3023 @@ | |||
"lockfileVersion": 1, | |||
"requires": true, | |||
"dependencies": { | |||
"jest": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there shouldn't be any diffs in this package-lock; likely from a npm install
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#1329 (comment) as well!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rectified the changed package-lock.json
!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the wait
I should really have untagged this as good first issue given the amount of additions in #947 (oh well =X)
Thanks! And well done for a first issue : )
What is the purpose of this pull request? (put "X" next to an item, remove the rest)
• [ ] Documentation update
• [ ] Bug fix
• [ ] New feature
• [X] Enhancement to an existing feature
• [ ] Other, please explain:
What is the rationale for this request?
This change helps the user easily access the website after deploying it using the MarkBind CLI.
What changes did you make? (Give an overview)
Github supports custom domains through the use of
CNAME
. However, to do so a file calledCNAME
must be placed in the root of the remote repository where we are deploying (the file is automatically created if one sets a manual deploy url through the github repo itself). Hence whenmarkbind deploy
is run, the following additional steps are done:CNAME
file in theorigin
remote with external librarysimple-git
.repo
fromsite.json
, otherwise using the remote itself.Provide some example code that this change will affect:
Testing instructions:
markbind init
.markbind deploy
and check the url the website is deployed at.Proposed commit message: (wrap lines at 72 characters)
Log website URL upon deployment